Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Allow facet button font weight to be set independenly #3476

Open
wants to merge 1 commit into
base: release-8.x
Choose a base branch
from

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Dec 17, 2024

No description provided.

@jcoyne jcoyne added this to the 8.x milestone Dec 17, 2024
@jrochkind
Copy link
Member

jrochkind commented Dec 17, 2024

I'm curious the the "theory" of what is being variabilized lately, it seems like an awful lot?

I don't think the goal to make pretty much every right-hand value in the CSS a variable? What is the goal or target of what to make a variable and what not?

In what ways/circumstances is it preferable to allow customization via CSS variable instead of an ordinary CSS override? In some cases a CSS override is not feasible, but I think in many cases where it is we're still adding CSS variables? (But I could be wrong?) And of course if a value is used more than once and has to match in all the places, CSS variables can provide a great solution there to allow them to be changed together, but that also doesn't seem to describe many of these new variables.

@jcoyne
Copy link
Member Author

jcoyne commented Dec 17, 2024

In what ways/circumstances is it preferable to allow customization via CSS variable instead of an ordinary CSS override?

I prefer setting variables over overriding. It seems clearer to me what the intention is.

What is the goal or target of what to make a variable and what not?

I have been targeting the things that we need for local customization at Stanford.

@jrochkind
Copy link
Member

jrochkind commented Dec 17, 2024

Cool, thanks.

I guess I think a proliferation of CSS variables makes the CSS a lot harder to debug and understand (I have experienced this), so prefer putting them in the shared codebase only when there is some specific reason -- like the value is used in multiple places that need to match, or an ordinary CSS override would be cumbersome -- rather than to put them in any place that anyone ever might want to customize.

If we do this for any place that any local app ever wants to customize, then nearly every right-hand side of a CSS rule anywhere could be a variable, right? I don't think that is the intention of CSS variables, and I think it makes the CSS confusing to work with.

Just my opinion! Curious what any others who work with Blacklight CSS may think. I realize we've already merged quite a few of these, I didn't totally realize unti now that we were doing it on the principle of universally every place that an institution wanted to customize.

Alternatlively, If this is the way we are doing it, I guess I could start PR'ing a variable into every place I want to override too, would consider it if that's the way BL is going to be designed, I want to go with the flow!

@jcoyne
Copy link
Member Author

jcoyne commented Dec 17, 2024

@jrochkind I've been taking bootstrap as inspiration. Almost everything in Bootstrap is using a variable like this. https://getbootstrap.com/docs/5.3/customize/css-variables/#component-variables

Here's how I'm customizing the facets:

      .sidenav {
        --bl-facet-limit-body-padding: 0.5rem;
        --bl-facet-margin-bottom: 0.5rem;
        --bl-facet-active-item-color: rgb(var(--bs-success-rgb));
      }

@jrochkind
Copy link
Member

OK if others feel the same, that's fine. I may start making PR's for my customizations too! You're doing each of these into main and also duplicated into 8.x?

@jcoyne
Copy link
Member Author

jcoyne commented Dec 18, 2024

@jrochkind this one is only going into 8.x, because in 9.x we've moved to accordion based facets, and we don't need custom config. Otherwise, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants